Move event-listeners-view in the modules folder, load it with require and detach it from anything in debugger-controllers.

Renaming because this first patch will also introduce my basic flux-inspired lib for interactions between stores and views. This will be the groundwork the rest of the refactoring will be based on for this first pass.

preliminary try push to see if this works: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4abe1ed86804

This is the first patch which introduces "fluxify", our intermediate solution for refactoring the debugger. In the future I'd like to use a real state management solution like https://github.com/gaearon/redux, but we need a first pass of refactoring first. EventListenersView has been refactored out into it's own isolated module. It's a good starting point because there's only small place that needs to change.

I ran a preliminary try run yesterday and it works on linux at least: https://treeherder.mozilla.org/#/jobs?repo=try&revision=52d269f39940

removed some unnecessary code

Comment on attachment 8630493 [details] [diff] [review] 1177891.patch Review of attachment 8630493 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/browser-loader.js @@ +14,5 @@ > + * > + * Another very important feature of this loader is that it *only* > + * deals with modules loaded from under `baseURI`. Anything loaded > + * outside of that path will still be loaded from the devtools loader, > + * so all system modules are still shared and cached across instances. Nice.

Added some more minor comments

bug 1181646 adds React, which will definitely land before this, and I went ahead and made the BrowserLoader automatically load the dev version of React if developing locally. BrowserLoader will handle all good things!

Comment on attachment 8631181 [details] [diff] [review] 1177891.patch Review of attachment 8631181 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/debugger-controller.js @@ +102,5 @@ > Cu.import("resource:///modules/devtools/VariablesView.jsm"); > Cu.import("resource:///modules/devtools/VariablesViewController.jsm"); > Cu.import("resource:///modules/devtools/ViewHelpers.jsm"); > > +Cu.import("resource:///modules/devtools/debugger/browser-loader.js"); Please, please. No more jsm!

Comment on attachment 8631181 [details] [diff] [review] 1177891.patch Review of attachment 8631181 [details] [diff] [review]: ----------------------------------------------------------------- If you plan to open tools within a webpage, shouldn't we try to use a pure-web loader? SDK loader uses Components.* and won't be able to run in a tab. Having said that, this is a possible incremental step to get there. ::: browser/devtools/debugger/browser-loader.js @@ +60,5 @@ > + const mainModule = loaders.Module(baseURI, joinURI(baseURI, "main.js")); > + const mainLoader = loaders.Loader(opts); > + > + return { > + loader: mainLoader, Shouldn't we only expose require? ::: browser/devtools/debugger/debugger-controller.js @@ +102,5 @@ > Cu.import("resource:///modules/devtools/VariablesView.jsm"); > Cu.import("resource:///modules/devtools/VariablesViewController.jsm"); > Cu.import("resource:///modules/devtools/ViewHelpers.jsm"); > > +Cu.import("resource:///modules/devtools/debugger/browser-loader.js"); Please, please. No more jsm!

(In reply to Alexandre Poirot [:ochameau] from comment #9) > Comment on attachment 8631181 [details] [diff] [review] > 1177891.patch > > Review of attachment 8631181 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/devtools/debugger/debugger-controller.js > @@ +102,5 @@ > > Cu.import("resource:///modules/devtools/VariablesView.jsm"); > > Cu.import("resource:///modules/devtools/VariablesViewController.jsm"); > > Cu.import("resource:///modules/devtools/ViewHelpers.jsm"); > > > > +Cu.import("resource:///modules/devtools/debugger/browser-loader.js"); > > Please, please. No more jsm! This isn't a jsm, but I'm assuming you mean no more Cu.import. This is the only place where I don't know what else we should do. We need to bootstrap ourselves to even *get* a loader. What should the pattern be? The current pattern is the load the devtools loader with a similar Cu.import. Is there another way to do it?

(In reply to Alexandre Poirot [:ochameau] from comment #10) > Comment on attachment 8631181 [details] [diff] [review] > 1177891.patch > > Review of attachment 8631181 [details] [diff] [review]: > ----------------------------------------------------------------- > > If you plan to open tools within a webpage, shouldn't we try to use a > pure-web loader? > SDK loader uses Components.* and won't be able to run in a tab. > > Having said that, this is a possible incremental step to get there. Yep, the only way I can keep any of this afloat is to do it in small steps. Right now the most important this is to clean up our code and start doing "React-ish" things. The main thing with web loaders is that ES6 modules are not available yet. That means we'd have to use RequireJS if we don't want a compile step, and I find it hard to use and worse than CommonJS (not to mention everything already uses CommonJS). I'd be fine with a compilation step, and use something like webpack to convert CommonJS modules into files that can be loaded on the web. This gets us writing all of our frontend files using CommonJS. There's nothing specific about this loader that the code cares about. In the future we could drop this loader completely and use a precompile step with webpack, or just change the CommonJS imports into ES6 imports when we land modules. > ::: browser/devtools/debugger/browser-loader.js > @@ +60,5 @@ > > + const mainModule = loaders.Module(baseURI, joinURI(baseURI, "main.js")); > > + const mainLoader = loaders.Loader(opts); > > + > > + return { > > + loader: mainLoader, > > Shouldn't we only expose require? This patch is a WIP, so I might do that. I think I needed to expose the loader so that you could unload all the modules when you are refreshing the page. I can't remember exactly, I'll think about it again though.

(In reply to James Long (:jlongster) from comment #11) > (In reply to Alexandre Poirot [:ochameau] from comment #9) > > Comment on attachment 8631181 [details] [diff] [review] > > 1177891.patch > > > > Review of attachment 8631181 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: browser/devtools/debugger/debugger-controller.js > > @@ +102,5 @@ > > > Cu.import("resource:///modules/devtools/VariablesView.jsm"); > > > Cu.import("resource:///modules/devtools/VariablesViewController.jsm"); > > > Cu.import("resource:///modules/devtools/ViewHelpers.jsm"); > > > > > > +Cu.import("resource:///modules/devtools/debugger/browser-loader.js"); > > > > Please, please. No more jsm! > > This isn't a jsm, but I'm assuming you mean no more Cu.import. This is the > only place where I don't know what else we should do. We need to bootstrap > ourselves to even *get* a loader. What should the pattern be? The current > pattern is the load the devtools loader with a similar Cu.import. Is there > another way to do it? Oh right, I was misreading this code. I though you still had Loader.jsm loaded in this scope, but as you are replacing it completely by browser-loader.js, that's fine! Yes we will keep having to do one Cu.import, for the Loader. But that's all.

Now we're getting somewhere! I think I've landed on a good first version of a library that will help us migrate to a flux-style architecture. But even better than flux: it paves the way for redux-style `(state, action) -> newState` style update functions. I'll write more about this later, but I'd like to start finally landing some refactoring patches. (the whole lib is so simple, just 137 lines, and manages flow of data). I refactored the event listeners view & controller more aggressively this time around. Instead of just moving views into this, I'm also going to start breaking up `debugger-controller.js` and converting them into "stores". The event listeners is fully based on the new architecture now (a good feature to start with, as it's rather isolated). My refactoring allows me to fully reuse a lot of complex business logic still. It's still a lot of just moving code around and making it more modular. There are places I need to refactor more deeply though (or simply want to), like breakpoints. No review on this yet, need to make one last clean up pass but I really want to push this to try. All tests passing locally.

Forgot to pull in latest fx-team changes before pushing. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4c2c9a68224

You reviewed this before, but I've changed it a lot. It should be ready for final review now. The dispatcher system is much more stabilized and ready to be used for the rest of the debugger refactoring. Mainly, look at tests, and `modules/utils.js` and the `asPaused` function. It should run fine on try, but I'll wait for the final review to do that. There will probably be some changes. Notably, I moved all the debugger mochitests into `tests/mochitests` and created a `test/unit` folder so I can run xpcshell tests there. The xpcshell tests cover the fluxify dispatcher system right now, but I'm excited about the possibility of testing more of the debugger frontend there. Any of the stores and action creators can be tests there, since they don't touch the DOM, and when we use React for components we can even do a lot of UI testing there.

This is the part of the patch which moves the mochitests to the new folder. I'll probably commit these as a single patch (a few of my tests changes got wrapped up in here) but separating to make review easier.

Hm, our review system doesn't show renames in the review process. Not sure if that's my fault or not. Anyway, look at the raw patch and you'll see the renames.

Argh, forgot to clean up one thing.

Comment on attachment 8652475 [details] [diff] [review] 1178609-move-tests.patch Review of attachment 8652475 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/test/mochitest/head.js @@ +60,5 @@ > > // Import the GCLI test helper > let testDir = gTestPath.substr(0, gTestPath.lastIndexOf("/")); > testDir = testDir.replace(/\/\//g, '/'); > testDir = testDir.replace("chrome:/mochitest", "chrome://mochitest"); wut... @@ +1193,5 @@ > + return new Promise(resolve => { > + dispatcher.dispatch({ > + // Normally we would use `services.WAIT_UNTIL`, but use the > + // internal name here so tests aren't forced to always pass it > + // in Nit: properly punctuate w/ periods

Argh I lost a bunch of review comments from the second part somehow :(

Comment on attachment 8652478 [details] [diff] [review] 1177891.patch Review of attachment 8652478 [details] [diff] [review]: ----------------------------------------------------------------- It looks like all the new files are missing the license header and "use strict";`. Let's move browser/devtools/debugger/modules/fluxify to browser/devtools/fluxify. Looks good overall, mostly nits. ::: browser/devtools/debugger/browser-loader.js @@ +32,5 @@ > + * outside of that path will still be loaded from the devtools loader, > + * so all system modules are still shared and cached across instances. > + * An exception to this is anything under > + * `browser/devtools/shared/browser`, which is where shared libraries > + * live that should be evaluated in a browser environment. Does this mean that we load a copy of these shared modules for each BrowserLoader? Because they rely on window/document/etc globals? (boo implicit dependencies) ::: browser/devtools/debugger/modules/fluxify/bindActionCreators.js @@ +14,5 @@ > + * @param {function} dispatch > + */ > +function bindActionCreators(actionCreators, dispatch) { > + let actions = {}; > + for (let k in actionCreators) { for (let k of Object.keys(actionCreators)) { ... } ::: browser/devtools/debugger/modules/fluxify/dispatcher.js @@ +6,5 @@ > +} > + > +function compose(...funcs) { > + return funcs.reduceRight((composed, f) => f(composed)); > +} Move these to DevToolsUtils.js and give them proper comments. ::: browser/devtools/debugger/modules/fluxify/thunkMiddleware.js @@ +7,5 @@ > +function thunkMiddleware({ dispatch, getState }) { > + return next => action => { > + return typeof action === 'function' ? > + action(dispatch, getState) : > + next(action); Double quotes. Also, preferred indentation for these is like so: return typeof action === "function" ? action(dispatch, getState) : next(action); ::: browser/devtools/debugger/modules/fluxify/waitUntilService.js @@ +28,5 @@ > + if (request.predicate(action)) { > + // Go ahead and remove it from the pending queue before > + // running it because it may call `dispatch` which runs this > + // service again > + pending = pending.filter(p => p !== request); http://accidentallyquadratic.tumblr.com/ Additionally, modifying the binding whose value you are currently iterating (while safe and not reentrant in this particular case) is pretty confusing and forces the reader to read the code extra hard. Instead let's take this approach: 1. Partition the pending list into two new lists: stillPending and ready. 2. Run each request in the ready list. 3. Rebind pending to stillPending's value. Feel free to early exit if the ready list is empty and avoid creating the stillPending list completely, if you want. I imagine this code isn't very hot though becuase you're fine doing an O(n) search on every action anyways. ::: browser/devtools/debugger/modules/stores/event-listeners.js @@ +29,5 @@ > +// our app state, we don't care about tracking them. Eventually we > +// could create an RDP service that internally tracks this sort of > +// state. > +let _fetchingListeners = false; > +let _refetchListeners = false; This seems like the kind of functionality that should be wrapped up in a decorator/higher-order-function and applied to any kind of fetching function rather than kept as global state. @@ +69,5 @@ > + const response = yield rdpInvoke(gThreadClient, gThreadClient.eventListeners); > + > + // Make sure all the listeners are sorted by the event type, since > + // they're not guaranteed to be clustered together. > + response.listeners.sort((a, b) => a.type > b.type ? 1 : -1); Let's pull this comparator out and give it a proper function definition so we aren't forced to read about how listeners are sorted everytime we read about getting listeners. Also, check for == first and return 0 in that case. Checking == is often very cheap (pointer comparison), but with > and < you always have to actually iterate characters in the string. ::: browser/devtools/debugger/test/unit/test_dispatcher.js @@ +1,1 @@ > +loadSubScript(getFileUrl("stores-for-testing.js")); 1. Why loadSubScript? Can't we use some kind of require? Or if we really have to, test-local JSMs are pretty easy to make[0] and are an improvement over loadSubScript. [0] https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Adding_to_your_test 2. All these fluxify specific tests (eg not stores/middlewares/etc specifically used by the debugger) should go under the fluxify directory.

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #23) > Comment on attachment 8652478 [details] [diff] [review] > 1177891.patch > > Review of attachment 8652478 [details] [diff] [review]: > ----------------------------------------------------------------- > > It looks like all the new files are missing the license header and "use > strict";`. > > Let's move browser/devtools/debugger/modules/fluxify to > browser/devtools/fluxify. Done, I actually moved it into browser/devtools/shared, and also moved browser-loader.js there, since it seems more appropriate in the shared folder. > > Looks good overall, mostly nits. > > ::: browser/devtools/debugger/browser-loader.js > @@ +32,5 @@ > > + * outside of that path will still be loaded from the devtools loader, > > + * so all system modules are still shared and cached across instances. > > + * An exception to this is anything under > > + * `browser/devtools/shared/browser`, which is where shared libraries > > + * live that should be evaluated in a browser environment. > > Does this mean that we load a copy of these shared modules for each > BrowserLoader? Because they rely on window/document/etc globals? (boo > implicit dependencies) Yes, but note it's basically not any different than how we would do things now. I think the web audio tool uses d3, and right now we would just load it as a normal <script> tag. All this does is lets us still use `require` so everything is consistent, but it's loaded the same way as a <script> tag which is how we do things now (well, sort of, it's loaded like a UMD library in it's own scope so it needs to export its interface instead of sticking it on window). Note that it's not anything in shared, just anything under the specific directory `shared/content` (it used to be `shared/browser`. If we really care to remove the window implicit dependency we can make that happen then move it to just `shared`. But really, the window is just part the browser builtin API. Just like we have "Components" in chrome code, node has "process" and other objects. While window is larger than it should be (I wish it weren't like this), going against this means we'll really never be able to use 3rd party libs. With the browser require, each module is loaded in a separate scope so you have to be really intentional about mutating the window object (the only way is literally `window.foo = 5`) and easy to enforce/avoid. React really needs to be loaded in the window it's working with. It does a lot of work to normalize events, and event objects all have a reference to the window instance and React uses this for optimizations like event delegation. Having objects flowing through a single React instance that all come from different windows is hard right now. The main problem is we use iframes, and there is actually discussion about how to use React across iframes & web workers so we might be above to leverage that in the future. > > ::: browser/devtools/debugger/modules/fluxify/bindActionCreators.js > @@ +14,5 @@ > > + * @param {function} dispatch > > + */ > > +function bindActionCreators(actionCreators, dispatch) { > > + let actions = {}; > > + for (let k in actionCreators) { > > for (let k of Object.keys(actionCreators)) { ... } done > ::: browser/devtools/debugger/modules/fluxify/dispatcher.js > @@ +6,5 @@ > > +} > > + > > +function compose(...funcs) { > > + return funcs.reduceRight((composed, f) => f(composed)); > > +} > > Move these to DevToolsUtils.js and give them proper comments. done > > ::: browser/devtools/debugger/modules/fluxify/thunkMiddleware.js > @@ +7,5 @@ > > +function thunkMiddleware({ dispatch, getState }) { > > + return next => action => { > > + return typeof action === 'function' ? > > + action(dispatch, getState) : > > + next(action); > > Double quotes. Also, preferred indentation for these is like so: > > return typeof action === "function" > ? action(dispatch, getState) > : next(action); done. is that in a style guide somewhere, or do people just differ on that? I don't usually like leading tokens. > > ::: browser/devtools/debugger/modules/fluxify/waitUntilService.js > @@ +28,5 @@ > > + if (request.predicate(action)) { > > + // Go ahead and remove it from the pending queue before > > + // running it because it may call `dispatch` which runs this > > + // service again > > + pending = pending.filter(p => p !== request); > > http://accidentallyquadratic.tumblr.com/ > > Additionally, modifying the binding whose value you are currently iterating > (while safe and not reentrant in this particular case) is pretty confusing > and forces the reader to read the code extra hard. > > Instead let's take this approach: > > 1. Partition the pending list into two new lists: stillPending and ready. > 2. Run each request in the ready list. > 3. Rebind pending to stillPending's value. > > Feel free to early exit if the ready list is empty and avoid creating the > stillPending list completely, if you want. I imagine this code isn't very > hot though becuase you're fine doing an O(n) search on every action anyways. Good catch. I wrote that in the last minute. I did pretty much what you said, except reversed #2 and #3. The point is that if the handler does another dispatch synchronously (a completely valid use case) that handler needs to not be in the pending list anymore already. The code is almost exactly as you describe though. And yeah, this isn't hot code at all. It'll probably only be used a few times, and only when certain network requests happen to wait until it succeeds. I expect there will only be 1 or 2 actions in the pending list every at a time. Still, now the whole thing is O(n). > > ::: browser/devtools/debugger/modules/stores/event-listeners.js > @@ +29,5 @@ > > +// our app state, we don't care about tracking them. Eventually we > > +// could create an RDP service that internally tracks this sort of > > +// state. > > +let _fetchingListeners = false; > > +let _refetchListeners = false; > > This seems like the kind of functionality that should be wrapped up in a > decorator/higher-order-function and applied to any kind of fetching function > rather than kept as global state. Yeah, this was just copy-and-pasted code. It was basically a singleton before. But this kind of code should not exist. I got this working before I had the waitUntil service, now I should be able to express this intention a lot more elegantly (just another type of action, and get rid of all these custom variables). > > @@ +69,5 @@ > > + const response = yield rdpInvoke(gThreadClient, gThreadClient.eventListeners); > > + > > + // Make sure all the listeners are sorted by the event type, since > > + // they're not guaranteed to be clustered together. > > + response.listeners.sort((a, b) => a.type > b.type ? 1 : -1); > > Let's pull this comparator out and give it a proper function definition so > we aren't forced to read about how listeners are sorted everytime we read > about getting listeners. Also, check for == first and return 0 in that case. > Checking == is often very cheap (pointer comparison), but with > and < you > always have to actually iterate characters in the string. Ok (note most of that code was copy-and-pasted too, though I am trying to clean it up some) > > ::: browser/devtools/debugger/test/unit/test_dispatcher.js > @@ +1,1 @@ > > +loadSubScript(getFileUrl("stores-for-testing.js")); > > 1. Why loadSubScript? Can't we use some kind of require? Or if we really > have to, test-local JSMs are pretty easy to make[0] and are an improvement > over loadSubScript. > > [0] > https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell- > based_unit_tests#Adding_to_your_test I don't think I should use `Cu.import`, the stores are inherently stateful right now. In the future the stores will be stateless and immutable, but one step at a time. I'm refactoring the controllers to be stores but a lot of the code still depends on mutability. Anyway, I need to import a fresh instance of these stores for each test, hence `loadSubScript`. (Also `loadSubScript` was used in other tests, that's actually the main reason I used it, but importing a fresh instance is another good reason) > > 2. All these fluxify specific tests (eg not stores/middlewares/etc > specifically used by the debugger) should go under the fluxify directory. Yeah, that's true. If I throw in a directory with xpcshell.ini it will automatically be picked up by everything (try server, etc)? I'll post a new patch tomorrow. Thanks for the great review!

FWIW, I'd prefer browser/devtools/fluxify to browser/devtools/shared/fluxify because I don't like the idea of specifying things that can or cannot be shared with a sub folder. If it were up to me, we would just put everything from browser/devtools/shared in browser/devtools. Its worth getting feedback from others on this, perhaps.

(In reply to James Long (:jlongster) from comment #24) > (In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #23) > > @@ +1,1 @@ > > > +loadSubScript(getFileUrl("stores-for-testing.js")); > > > > 1. Why loadSubScript? Can't we use some kind of require? Or if we really > > have to, test-local JSMs are pretty easy to make[0] and are an improvement > > over loadSubScript. > > > > [0] > > https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell- > > based_unit_tests#Adding_to_your_test > > I don't think I should use `Cu.import`, the stores are inherently stateful > right now. In the future the stores will be stateless and immutable, but one > step at a time. I'm refactoring the controllers to be stores but a lot of > the code still depends on mutability. Anyway, I need to import a fresh > instance of these stores for each test, hence `loadSubScript`. > > (Also `loadSubScript` was used in other tests, that's actually the main > reason I used it, but importing a fresh instance is another good reason) This should be documented in a comment then.

What do you think about comment 25, jryans? My new stuff is in a folder called "fluxify" and will be shared across tools. Should it go in `browser/devtools/shared` or just `browser/devtools`? I like the shared folder because we have a lot of files that are not tool-specific and it doesn't clutter up the `browser/devtools` directory, personally.

(In reply to James Long (:jlongster) from comment #27) > What do you think about comment 25, jryans? My new stuff is in a folder > called "fluxify" and will be shared across tools. Should it go in > `browser/devtools/shared` or just `browser/devtools`? I like the shared > folder because we have a lot of files that are not tool-specific and it > doesn't clutter up the `browser/devtools` directory, personally. I prefer an explicit "shared" folder as well. It seems like a nice way to say "this is a general utility which many tools may find useful". Then the remaining directories in browser/devtools are actual tools, not a mix of libs and tools. The files that live at "browser/devtools/*.js" today are more like "main entry points" to all the tools (like main.js), so that seems like a natural place for those to be. Perhaps I am just used to how things have been for a while, though. :) Either way is fine as far the file move goes, so no concern there.

updated

updated

another try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=baa618b6d9cd

Ok, final try. This one looks like it's going to be good. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f06a61085076

Renaming patches to make the order clearer

https://hg.mozilla.org/integration/fx-team/rev/038b3c7ea328 https://hg.mozilla.org/integration/fx-team/rev/80d8ce7d7280

https://hg.mozilla.org/mozilla-central/rev/038b3c7ea328 https://hg.mozilla.org/mozilla-central/rev/80d8ce7d7280